Skip to content

Conversation

@pcc
Copy link
Contributor

@pcc pcc commented Jun 12, 2025

This is useful for debugging ThinLTO issues.

pcc added 2 commits June 12, 2025 16:05
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1

[skip ci]
@pcc pcc requested a review from teresajohnson June 12, 2025 23:05
Created using spr 1.3.6-beta.1
Copy link
Contributor

@teresajohnson teresajohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a caveat somewhere (either in printed usage message or in a comment) that this won't work for local linkage symbols (I suppose the user could give the "file:" prefix but that won't work if -funique-internal-linkage-names was specified etc). Can you also add a test?

pcc added 2 commits June 13, 2025 13:49
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@pcc
Copy link
Contributor Author

pcc commented Jun 13, 2025

This needs a caveat somewhere (either in printed usage message or in a comment) that this won't work for local linkage symbols (I suppose the user could give the "file:" prefix but that won't work if -funique-internal-linkage-names was specified etc).

I'm not sure that is worth it. The intent is that users of these development tools will refer to the source code. And if you read the source code you'll see the function name getGUIDAssumingExternalLinkage which tells you what you need to know.

Can you also add a test?

Done.

@teresajohnson
Copy link
Contributor

This needs a caveat somewhere (either in printed usage message or in a comment) that this won't work for local linkage symbols (I suppose the user could give the "file:" prefix but that won't work if -funique-internal-linkage-names was specified etc).

I'm not sure that is worth it. The intent is that users of these development tools will refer to the source code. And if you read the source code you'll see the function name getGUIDAssumingExternalLinkage which tells you what you need to know.

At least a comment in the code would be good. A variety of people end up using these tools for tests, and I could see someone getting confused as to why the guid doesn't match what's e.g. in the ThinLTO index. For that understanding you'd have to read more than just what this source file is calling.

Copy link
Contributor

@teresajohnson teresajohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm otherwise

Created using spr 1.3.6-beta.1
@pcc
Copy link
Contributor Author

pcc commented Jun 13, 2025

At least a comment in the code would be good. A variety of people end up using these tools for tests, and I could see someone getting confused as to why the guid doesn't match what's e.g. in the ThinLTO index. For that understanding you'd have to read more than just what this source file is calling.

I added a comment.

@pcc pcc changed the base branch from users/pcc/spr/main.llvm-lto2-add-print-guid-subcommand to main June 13, 2025 21:35
@pcc pcc merged commit 3afc2be into main Jun 13, 2025
3 checks passed
@pcc pcc deleted the users/pcc/spr/llvm-lto2-add-print-guid-subcommand branch June 13, 2025 21:35
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jun 13, 2025
This is useful for debugging ThinLTO issues.

Reviewers: teresajohnson

Reviewed By: teresajohnson

Pull Request: llvm/llvm-project#143992
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
This is useful for debugging ThinLTO issues.

Reviewers: teresajohnson

Reviewed By: teresajohnson

Pull Request: llvm#143992
akuhlens pushed a commit to akuhlens/llvm-project that referenced this pull request Jun 24, 2025
This is useful for debugging ThinLTO issues.

Reviewers: teresajohnson

Reviewed By: teresajohnson

Pull Request: llvm#143992
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants